-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HTTP2] Make HTTPVersion public and set to .automatic by default #473
[HTTP2] Make HTTPVersion public and set to .automatic by default #473
Conversation
) | ||
} | ||
|
||
// TODO: make public | ||
internal init( | ||
public init( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH: I don't think we should add yet another initializer for HTTPClient.Configuration. I think we should deprecate all of them, except for init()
. In init()
we set all default values and then the user can change the config, since it is just a struct. We have precedent for this move in NIOSSL.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand you correct, a user will then only be able to change the config by create a temporary variable, initialising it with our default config, mutating it and passing it to the HTTPClient
initialiser, right? This would be a rather big change because we would need to update all our tests if we deprecate all other inits. I would rather do that in another PR if we would like to do that before merging this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @Lukasa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also think about adding an extension to HTTPClient.Configuration
so we can change the configuration through daisy chaining like I have added in RSocket:
extension HTTPClient.Configuration {
/// Creates a copy and updates the property described by `keyPath` with the given `newValue`
/// - Returns: updated configuration
public func set<Value>(_ keyPath: WritableKeyPath<Self, Value>, to newValue: Value) -> Self {
var copy = self
copy[keyPath: keyPath] = newValue
return copy
}
}
This would ease the transition because you probably end up with the same amount of lines as with an init
and a user does not have to add a temporary variable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we are already on it, what about nested types HTTPClient.Configuration
uses e.g. HTTPClient.Configuration.ConnectionPool
and HTTPClient.Configuration.Timeout
? They will probably suffer from the same problem, but on a much smaller scale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm inclined to tackle this in a separate PR. For now we don't need to add a new init for this feature, we can just exclude it from the pre-existing inits.
Fixes #28. |
a061d78
to
92444bd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! It's great that we can finally expose this!
458bf4b
to
5bf0bf6
Compare
This PR does two things:
HTTPVersion
publicHTTPVersion
to.automatic
by defaultHTTP/2 will then be used if we connect to a server with HTTPS and the server supports HTTP/2, otherwise we will continue to use HTTP/1.
A user will still be able to disable HTTP/2 if they want to by setting
HTTPVersion
to.http1Only
onHTTPClient.Configuration
e.g.: